Is this a security bug that the collision is undetected?

The collisions can happen with some very small but non-zero probability. If a program is unlucky to hit the collision, the string literals will be messed up and the behavior of the program will be impacted in interesting ways. This can be used by adversaries to contribute an innocently looking string to an open-source project where the string is carefully crafted to cause a collision with some other critical string.

There are a few places in the compiler where we use hashes and collisions could lead to program behavior changes. These are part of our security reviews in the past and this will be included in our next one. Thus far nothing has risen to the level of forcing extra checking. There is an increase in non-cryptographic hashing support though and I will make sure that is included in the next security review. @RikkiGibson

Right, XXH128 is non-cryptographic hash, so one should assume that it is easy to find collisions. If we are going to keep the current implementation for this feature, I would be curious to know how we are justifying that it is ok.

Understand the concern here. But you can create similar collisions today at much lower level places in the compiler by creating collisions with file contents. The use of non-crypto hashes was approved at that layer too (was in fact suggested by security team). The general reasoning is that once you can control code being submitted to the project you have lots of avenues for subverting expectations.

The general reasoning is that once you can control code being submitted to the project you have lots of avenues for subverting expectations.

In the open-source world, it is important that the project maintainers are able to review the changes with confidence.

For example, let's say that this feature gets enabled for some part of OpenTelemetry project and somebody submits a PR that includes a code like this:

void DiagnosticThatIsTurnedOffByDefault()
{
   // Random unique string to help us identify blah
   DoStuff("ruroiodffdshofps09rw403492049358302849052peldfjskdfshgfdshiofofjsekljhkdkjafhkds3");
}

How can the maintainer of the OpenTelemetry project tell that this change is actually planting a backdoor into an unrelated code due to a hash collision?

xxHash128 is also used to identify source documents in the IDE currently, as well as to identify interceptable calls in source code. We consider it collision-safe for these purposes. See also the Examples at the bottom of this doc: https://web.archive.org/web/20240223124645/https://fossies.org/linux/xxHash/tests/collisions/README.md

@CyrusNajmabadi in case you have further thoughts.

xxHash128 is also used to identify source documents in the IDE currently

I would not be worried about the IDE case from the security angle. If somebody crafts a file name with a hash collision and it makes the IDE to misbehave or crash, it is not a big deal. (You may be scratching your head for a while if you are tasked with investigating the crash report.)

FWIW, I think it should be straightforward to add collision detection, it just didn't seem straightforward to test that, but I can look into that more.

#77061

Actually I think hash collisions might not be a problem - #77061 (comment)

We should make this happen if you plan to advertise this feature for broader use, and not just as a workaround for the few folks who run into the limit.

The plan is to use the current state of implementation, compiler only trick, to get feedback on the scenarios. We did a number of benchmarks on the work and didn't see meaningful differences that justified preemptively doing runtime work. Decided that we would ship it out as experimental, get real world feedback from teams like Bing, and then let that drive what we wanted the final state of the feature to be before we remove experimental lable

@davidwrighton, @DamianEdwards